Skip to content

Precommit: Format toolchain scripts - #8213

Open
Growl1234 wants to merge 4 commits into
deepmodeling:developfrom
Growl1234:precommit-bash
Open

Growl1234 wants to merge 4 commits into
deepmodeling:developfrom
Growl1234:precommit-bash

Conversation

@Growl1234

Copy link
Copy Markdown
Collaborator

This looks like quite a change mainly because indent is changed (default is tab, which is definitely unacceptable to me; 4 feels weird for a bash script).

I had to replace the multi-line CMake commands using backslash continuations (... \) with cmake_args arrays, because shfmt breaks them when a continued line is followed by a commented-out option.

@Growl1234 Growl1234 added the Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS label Oct 23, 2026

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore Git mode 100755 on toolchain/build_abacus_{aocc-aocl,gcc-aocl,gcc-mkl,gnu,intel}.sh. They change to 100644, so the documented direct commands and GNU/Intel/CUDA full-toolchain workflow cannot execute them from a fresh POSIX checkout. The quick workflow's local chmod masks this regression.

Non-blocking: Please add the exact validation commands/results and note any checks not run in the PR description. Its current text describes the formatting and array rewrite but contains no validation record.

Static review of 4f3b0a016f67cc769809b462bd40cbada32e9f5e: all 48 changed shell files parse at base and head with Bash 5.3.15. Installers, hooks and product tests were not run; CI results are unavailable.

Comment thread .pre-commit-config.yaml Resolved
@Growl1234

Copy link
Copy Markdown
Collaborator Author

Please restore Git mode 100755 on toolchain/build_abacus_{aocc-aocl,gcc-aocl,gcc-mkl,gnu,intel}.sh. They change to 100644, so the documented direct commands and GNU/Intel/CUDA full-toolchain workflow cannot execute them from a fresh POSIX checkout. The quick workflow's local chmod masks this regression.

Restored.

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed at 282bb24698b9c795611524d9589181d2330e7adc: the five Unix build scripts identified in the earlier review are tracked as 100755 again, matching your restoration update. The six executable-bit changes preserve file contents, and no new issue was found in this incremental static review. Verification was limited to Git metadata and source inspection; no builds, tests, or hooks were run.

Comment thread toolchain/build_abacus_aocc-aocl.sh Resolved
Comment thread toolchain/build_abacus_gcc-mkl.sh Resolved
Comment thread toolchain/build_abacus_gnu.sh Resolved
Comment thread toolchain/build_abacus_intel.sh Resolved
@ZhouXY-PKU

Copy link
Copy Markdown
Collaborator

@ABACUS-PR-BOT Please make sure there are no changes except format.

@ABACUS-PR-BOT ABACUS-PR-BOT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the full 49-file diff (89595dd29a26aec34d623518cadf736c17345f69 → 282bb24698b9c795611524d9589181d2330e7adc): it is not strictly formatting-only.

  • Adds the shfmt hook in .pre-commit-config.yaml; its repository-wide scope is already discussed here.
  • Rewrites five Linux CMake commands as arrays and adds quoting. Active options/order are preserved for ordinary single-field paths, but whitespace/glob handling changes; generated environment scripts and usage text also gain quotes. See the existing CMake discussion.
  • Changes toolchain/build_abacus_windows.sh from mode 100644 to 100755. The five Unix build-script modes match the base.

No unintended regression was found in the static comparison. The other syntax rewrites, including ${x//.} → ${x//./}, preserve the inspected command semantics. All 48 shell files passed base/head Bash --pretty-print -n parsing (96 parses), and exact-base/head git diff --check passed. Installers, builds, tests and shfmt were not run; runtime behavior and arbitrary environments remain unverified.

Comment thread toolchain/build_abacus_windows.sh Resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants